feat: schedule automatic undelegation on AML check failure#1300
feat: schedule automatic undelegation on AML check failure#1300Dodecahedr0x wants to merge 9 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 58 minutes and 23 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds an AML-triggered undelegation scheduling path. A new Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@magicblock-api/src/magic_validator.rs`:
- Around line 115-120: Wrap the await on self.0.schedule_undelegation(pubkey,
request.account).await in a tokio timeout (e.g.,
tokio::time::timeout(Duration::from_secs(...), ...)) so the call cannot hang
indefinitely; after awaiting the timeout, map a timeout error to the same
chainlink/committor error variant currently produced (the "committor response
channel closed" mapping) so both channel-closure and timeout produce the same
error path, and then continue to .and_then(|result| ...) as before; update
imports if necessary to include tokio::time::timeout and Duration and ensure the
timeout mapping converts into a String consistent with the existing
.map_err(|message| ...) flow.
In `@magicblock-chainlink/src/chainlink/fetch_cloner/tests.rs`:
- Around line 251-255: The mock server's blocking accept loop inside the
tokio::task::spawn_blocking worker (which calls listener.accept() in a for
0..expected_calls loop) can hang CI if fewer connections arrive; modify the
worker to enforce a hard timeout (e.g., 5s) for each accept or for the entire
loop: set the TcpListener to non-blocking (listener.set_nonblocking(true)) or
use Instant::now()/elapsed to track time, repeatedly try accept() with a short
sleep, and if the deadline is exceeded before receiving all expected_calls,
break and return an error (or panic) so the test fails fast instead of hanging
while the test awaits the worker join. Ensure the change is applied to the same
worker block that spawns the mock server and to the other similar instance
mentioned (around the other accept loop).
In `@magicblock-chainlink/tests/10_aml_undelegation.rs`:
- Around line 70-74: The mock risk-server worker can block forever on
listener.accept() inside the spawned closure (the worker) causing join() to
hang; change the worker loop to avoid blocking by calling
listener.set_nonblocking(true) (or use try_accept if available) and perform a
bounded retry loop with a short sleep, counting successful accepts until
expected_calls or until a configurable timeout elapses, then break and return an
error/result so the task completes; update the code that awaits the worker (the
join() site) to handle the early-return error case. Ensure you reference the
spawned closure (worker), listener.accept, expected_calls and the join()
awaiting the worker when making the change.
In `@magicblock-committor-service/src/committor_processor.rs`:
- Around line 302-309: The current intent_id() uses wall-clock nanoseconds which
can collide; replace it with a collision-free generator such as a monotonic
atomic counter or a UUID-based ID instead of SystemTime::now(). Specifically,
update the intent_id() function to read from a shared AtomicU64 (e.g.,
NEXT_INTENT_ID.fetch_add(1, Ordering::SeqCst)) or generate a UUID and
convert/encode it for ScheduledIntentBundle.id so each undelegation gets a
unique, monotonic or globally-unique ID; ensure the chosen generator is
initialized and accessible where intent_id() is called and that persisted
keys/status lookups use the same ID format.
In `@magicblock-committor-service/src/service.rs`:
- Around line 68-72: The ScheduleUndelegation variant embeds AccountSharedData
which causes CommittorService::try_send() to log full account bytes on
TrySendError::{Full,Closed}; change the logging to avoid dumping the account
payload by either redacting ScheduleUndelegation's Debug output or logging only
the pubkey and message type. Concretely, update the CommittorService::try_send()
error paths that currently log the CommittorMessage to instead pattern-match the
message and log a concise string for ScheduleUndelegation (e.g.,
"ScheduleUndelegation { pubkey: <pubkey> }") or implement a custom Debug/Display
for that enum variant that omits AccountSharedData; ensure references to
ScheduleUndelegation, CommittorMessage, AccountSharedData, Pubkey, and
TrySendError::Full/Closed are used to locate and change the relevant code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dd15f542-18ab-4785-a422-c9f5e2e1d1a1
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
magicblock-api/Cargo.tomlmagicblock-api/src/magic_validator.rsmagicblock-chainlink/Cargo.tomlmagicblock-chainlink/src/chainlink/errors.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/chainlink/mod.rsmagicblock-chainlink/tests/10_aml_undelegation.rsmagicblock-chainlink/tests/utils/test_context.rsmagicblock-committor-service/src/committor_processor.rsmagicblock-committor-service/src/service.rsmagicblock-committor-service/src/service_ext.rsmagicblock-committor-service/src/stubs/changeset_committor_stub.rstest-integration/test-chainlink/src/ixtest_context.rstest-integration/test-chainlink/src/test_context.rs
|
@Dodecahedr0x : can you please add some description as to when/why undelegation should be scheduled and automated. |
There was a problem hiding this comment.
I think this PR digresses from current committor flow, and imo there's no need in this deviation.
Delegation is an account state property, therefore it has to go through builtin program and modify delegated flags.
This makes flow similar to regular Intent scheduling one, in fact you do schedule an Intent youself.
- Validator detect account is malicious
- Validator sends TX in magic-program(new entrypoint, or an old one with commit)
- magic-program mutates account states correctly and schedules undelegation(
mark_account_as_undelegated) - CommittorService picks it up and executes as in current flow
This gets rid of new schedule_undelegation functionality completely as it will seamlessly integrates into current Committor logic. All is needed is new entrypoint in magic-program and TX sender
Current state bypasses all of that flow to which it belongs. This additionally lead to an amigious state where in AccountsDB account is delegated but on Base it is not. Maybe cloner will fix this(which I'm not sure about) after some updates, but this leaves window for an attack where in meantime it could be used by user.
| struct CommittorUndelegationScheduler(Arc<CommittorService>); | ||
|
|
||
| #[async_trait::async_trait] | ||
| impl UndelegationScheduler for CommittorUndelegationScheduler { |
There was a problem hiding this comment.
magic_validator.rs is already massive, let's extract this into separate file
| } | ||
| } | ||
|
|
||
| pub(crate) fn undelegation_intent_bundle( |
There was a problem hiding this comment.
This has to be created by magic-program which additionally would call mark_account_as_undelegated
| } | ||
| } | ||
|
|
||
| fn intent_id() -> u64 { |
There was a problem hiding this comment.
Intent ids are assigned by magic-program
|
@taco-paco I completely reworked the flow to use the regular commit path. However, it now clones and undelegates the account in a single transaction. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
programs/magicblock/src/utils/instruction_utils.rs (1)
82-84:⚠️ Potential issue | 🟠 MajorMark accounts at index 2..n as non-signers, not signers.
Line 83 marks each PDA as writable and signer with
AccountMeta::new(*pubkey, true), but the instruction spec (magicblock-magic-program-api/src/instruction.rs, line 57) documents accounts 2..n as[](read-only, non-signer). Since the builder signs with only the payer, non-payer accounts marked as signers will fail transaction sanitization in production.Suggested fix
for pubkey in &pdas { - account_metas.push(AccountMeta::new(*pubkey, true)); + account_metas.push(AccountMeta::new_readonly(*pubkey, false)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@programs/magicblock/src/utils/instruction_utils.rs` around lines 82 - 84, The for loop in instruction_utils.rs that processes PDAs (line 82-84) marks each account as both writable and signer using AccountMeta::new with true as the second parameter. However, according to the instruction specification in magicblock-magic-program-api/src/instruction.rs at line 57, accounts 2..n should be read-only, non-signers. Change the AccountMeta::new call to use an appropriate constructor method that marks these PDA accounts as read-only and non-signers instead, since only the payer is expected to sign the transaction and marking non-payer accounts as signers will cause transaction sanitization to fail in production.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test-integration/Makefile`:
- Around line 57-64: The test-aml target bypasses required program preparation
because the shared test target only runs chainlink-prep-programs when RUN_TESTS
is empty or contains chainlink or cloning. Since aml.devnet.toml requires
artifacts from ../target/deploy/miniv3/program_mini.so, ensure
chainlink-prep-programs is invoked before running aml tests. Either add aml to
the condition that triggers chainlink-prep-programs in the shared test target,
or explicitly invoke chainlink-prep-programs as a prerequisite step in the
test-aml target (similarly to how other test configurations handle program
preparation).
---
Outside diff comments:
In `@programs/magicblock/src/utils/instruction_utils.rs`:
- Around line 82-84: The for loop in instruction_utils.rs that processes PDAs
(line 82-84) marks each account as both writable and signer using
AccountMeta::new with true as the second parameter. However, according to the
instruction specification in magicblock-magic-program-api/src/instruction.rs at
line 57, accounts 2..n should be read-only, non-signers. Change the
AccountMeta::new call to use an appropriate constructor method that marks these
PDA accounts as read-only and non-signers instead, since only the payer is
expected to sign the transaction and marking non-payer accounts as signers will
cause transaction sanitization to fail in production.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b5b1a290-f756-4c8a-a7f2-c08af943c5a5
⛔ Files ignored due to path filters (1)
test-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
magicblock-account-cloner/src/lib.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/chainlink/mod.rsmagicblock-chainlink/src/cloner/errors.rsmagicblock-chainlink/src/cloner/mod.rsmagicblock-chainlink/tests/utils/test_context.rsmagicblock-magic-program-api/src/instruction.rsprograms/magicblock/src/clone_account/process_post_delegation_actions.rsprograms/magicblock/src/magicblock_processor.rsprograms/magicblock/src/schedule_transactions/mod.rsprograms/magicblock/src/schedule_transactions/process_schedule_cloned_undelegation.rsprograms/magicblock/src/utils/instruction_utils.rsprograms/magicblock/src/utils/mod.rsprograms/magicblock/src/utils/validation.rstest-integration/Cargo.tomltest-integration/Makefiletest-integration/configs/aml.devnet.tomltest-integration/test-aml/Cargo.tomltest-integration/test-aml/src/lib.rstest-integration/test-aml/tests/range_mock.rstest-integration/test-chainlink/Cargo.tomltest-integration/test-chainlink/src/ixtest_context.rstest-integration/test-chainlink/src/test_context.rstest-integration/test-chainlink/tests/ix_aml_undelegation.rstest-integration/test-runner/Cargo.tomltest-integration/test-runner/bin/run_tests.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@magicblock-account-cloner/src/lib.rs`:
- Around line 873-878: The panic messages in the match arms for
PostDelegationActionExecutorInstruction::ScheduleUndelegation are inverted and
misleading. These arms trigger when ScheduleUndelegation is unexpectedly
received, but the panic message says "expected schedule undelegation
instruction" which is backwards. Locate both occurrences of the
ScheduleUndelegation match arm and update each panic message to accurately
reflect that ScheduleUndelegation was received unexpectedly, and state what
instruction was actually expected instead (which should be Execute based on the
test expectations).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a7fc8003-2a37-466a-96e1-35961d05bf41
⛔ Files ignored due to path filters (1)
test-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
magicblock-account-cloner/src/lib.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/chainlink/mod.rs
Summary
Automatically schedules the undelegation of AML-flagged accounts. Currently, if an account fails the AML check, it is just not cloned, meaning it stays owned by the DLP on mainnet but can't be used in the ER. This PR automatically undelegates it.
Breaking Changes
Summary by CodeRabbit